Validate Variable key is a non-empty string in get() and set()#69606
Validate Variable key is a non-empty string in get() and set()#69606bramhanandlingala wants to merge 3 commits into
Conversation
potiuk
added
the
ready for maintainer review
|
thanks @potiuk @bugraoz93, @choo121600, @ephraimbuddy, @henry3260, @jason810496, @rawwar @kaxil @Lee-W |
|
@bugraoz93, @choo121600, @ephraimbuddy, @henry3260, @jason810496, @rawwar @kaxil @Lee-W @shahar1 request You all please review and approve this PR for merge |
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
First, kind request for next times - please do not tag multiple maintainers in pull requests without ensuring that all are ok with it. Maintainers here have their own priorities - so please consider it and respect their time. If you don't get an answer within a reasonable time, you may post on either of the Slack channels (#new-contributors / #contributors). Tagging 10 maintainers in the PR once a week is not the appropriate way to do so (also, some of the maintainers might not be domain experts of this specific area).
Also, please note that the original issue was marked as an AI-slop and closed (author spammed general Python advices as issues and PRs with no good use), so it already brings the question of real necessity on the table.
Putting the above aside, let's get to the point - the change itself doesn't address the problem it claims to solve (disclaimer - the following were drafted by AI, validated by myself):
-
Wrong layer: In Airflow 3, Dag authors should use
airflow.sdk.Variable-airflow.models.Variableis the deprecated compat path that warns and redirects to the SDK in task context. This PR validates only the deprecated entry point, whileairflow.sdk.Variable.set(None, ...)/.get(None)behave exactly as before. So the API we actually tell users to use gains nothing, and the two entry points now diverge. -
Behavior change:
Variable.get()misses raiseKeyError, andexcept KeyErroris a common user pattern. After this changeget(None)raisesValueErrorinstead - a user-facing contract change that can't ride in as a side effect (and would at least need a newsfragment). -
Inconsistency:
delete()is left unvalidated, so the class ends up half-validated.
The actual "problem" is also thin:set(None)already fails loudly (uglyIntegrityError, but it fails),get(None)raisesKeyError, and empty-string keys are already rejected by the API/UI on normal paths. That leaves a cosmetic error-message improvement on a deprecated path - not enough to justify the contract change.
Closing this one. If there's a real user report of hitting this, key validation could be reconsidered - but in the Task SDK class, applied consistently across get/set/delete.
3 participants
Description
While looking into #69595, I checked
Variable.set()andVariable.get()inairflow-core/src/airflow/models/variable.py, since they're two of the most commonly used public APIs in Airflow — called directly in DAG code all the time.Root cause:
Neither function validated the
keyargument before doing anything with it. InVariable.set(), the key flowed straight through to a database write. Thekeycolumn is declared NOT NULL with nonullable=True, soVariable.set(None, "x")reached the database before failing, surfacing a rawsqlalchemy.exc.IntegrityErrorwith a stack trace that doesn't say what actually went wrong.Variable.set("", "x")was worse — an empty string satisfies NOT NULL, so it silently succeeded and created a variable with an unusable empty-string key.Variable.get()had a milder version of the same gap.Variable.get(None)doesn't crash, but it queries the database for a variable literally namedNone, finds nothing, and raisesKeyError("Variable None does not exist.")— a message that reads like there's a real variable you're missing, not that the key itself was invalid.Fix:
Added the same check to the top of both methods, before any database or Task-SDK redirect logic runs:
This catches all three cases the issue describes —
None, empty string, and non-string values — with one consistent, descriptive error instead of a database exception or a misleadingKeyError.Didn't need to touch
setdefault()separately since it callsget()internally as its first line, so it inherits the same validation automatically. Added a test to confirm that rather than just assuming it.related: #69595